Remove legacy geo code from AggregationResultUtils #77702
Conversation
|
Pinging @elastic/ml-core (Team:ML) |
| Rectangle r = GeoTileUtils.toBoundingBox(key.toString()); | ||
| Polygon polygon = new Polygon( | ||
| new LinearRing( | ||
| new double[]{r.getMinLon(), r.getMaxLon(), r.getMaxLon(), r.getMinLon(), r.getMinLon()}, |
There was a problem hiding this comment.
Does the order matter here? It seems that this 2-D array is not a transposition of the 2-D array on the left-hand side.
There was a problem hiding this comment.
Probably it does no matter because they represent the same polygon.
|
I am not happy changing the output of the aggregation, maybe we should just replace all the *..getPreferredName() with local static strings? |
|
I am missing context for this change. It seems to me that the upstream format for encoding geo shapes is changing. The docs don't mention a change (yet?). E.g. Does that mean we get rid of the "Elasticsearch Type" and make it the "GeoJSON Type"? Do you have a full example to see the differences? As for "changing the output": Transform takes the output of the aggregation and produces index requests out of it. That means this basically only changes |
No really, our json parser is (maybe too) flexible and accepts both signatures, lowercase and camelcase. Therefore both types are supported. What are we moving to legacy is everything under org.elasticsearch.common.geo.builders.* (and I guess we should have deprecated it) in favour of the elasticsearch geo library. The issue is that those builders depend on JTS and we want to remove JTS from server. I think it is not necessary to make a breaking change but stop using the builders to get field names and just have them locally? |
|
ok, got it, so the change is backwards compatible.
I understand now what you mean. Sounds good to me. |
|
Done, added some static strings to replace the legacy objects. |
|
@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample |
|
@elasticmachine update branch |
💚 Backport successful
|
* master: (185 commits) Implement get and containsKey in terms of the wrapped innerMap (elastic#77965) Adjust Lucene version and enable BWC tests (elastic#77933) Disable BWC to upgrade to Lucene-8.10-snapshot Reenable MlDistributedFailureIT [DOCS] Fix typo for `script.painless.regex.enabled` setting value (elastic#77853) Upgrade to Lucene-8.10.0-snapshot-bf2fcb53079 (elastic#77801) [DOCS] Fix ESS install lead-in (elastic#77887) Resolve thirdparty gradle plugin artifacts from mavencentral (elastic#77865) Reduce the number of times that `LifecycleExecutionState` is parsed when running a policy. (elastic#77863) Utility methods to add and remove backing indices from data streams (elastic#77778) Use Objects.equals() instead of == to compare strings (elastic#77840) [ML] prefer least allocated model when a new node is added to the cluster (elastic#77756) Deprecate ignore_throttled parameter (elastic#77479) Improve LifecycleExecutionState parsing. (elastic#77855) [DOCS] Removes deprecated word from HLRC title. (elastic#77851) Remove legacy geo code from AggregationResultUtils (elastic#77702) Adjust SearchableSnapshotsBlobStoreCacheIntegTests.testBlobStoreCache (elastic#77758) Laxify SecureSM to allow creation of the JDK's innocuous threads (elastic#77789) [Test] Reduce concurrency when testing creation of security index (elastic#75293) Refactor metric PipelineAggregation integration test (elastic#77548) ... # Conflicts: # server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java
AggregationResultUtils use some code for serialising the results of some geo aggregations that is considered legacy and it will be removed in the future. We should move this code to use the elasticsearch geo library.
This approach uses the simpler way to move to the new libraries but unfortunately it changes the format of the serialisation slightly which I guess might be problematic for backwards compatibility.